-
Notifications
You must be signed in to change notification settings - Fork 724
Defer merging of common stanzas #11277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Defer merging of common stanzas #11277
Conversation
Currently we have achieve the following: - Stop merging, the merging function "endo" is id - CondTree are completly retained in bigger types such as libarry and executable We will need to do the following: - Allow merging in the accessor We broke: - A bunch of Read and Ord instances
This reverts commit 21636db.
We isolated the type TestSuiteStanza and the logic to infer test type
We isolated the BenchmarkStanza type and the logic to infer benchmark type.
We now use the WithImports type to tag imports
492ad9e to
d8a1fcc
Compare
This guarantees that unmerged internal representation is correct
|
Here are the benchmark results by timing the
|
|
I'm not sure why validate is used for benchmarking parser changes. Validate is likely dominated by completely unrelated factors (e. g. solver). Would it be possible to just download a bunch of cabal files from Hackage (see clc-stackage for a curated set of packages) and run only the parser on them? |
The options
I thought it would be important to test out whether I broke the solver by changing the representation to GPD too, which is why I tested everything :) |
these are tests, which is different from benchmarking. You seem to be using validate for benchmarking, which makes no sense to me, as explained above. On the other hand, using validate for tests may have some value although it is preferable to use the actual test like |
This is pretty much what the hackage-test test does. Do you think timing this alone would be more acceptable (compare to ./validate) even though it's not a benchmark? |
|
If timing hackage-tests would mean timing download times too, then no, it doesn't sound right. I don't know if you can arrange for separate download, which would make it more reasonable. Generally, if you want to have an estimate for performance footprint, you have to develop a benchmark, you can't just hijack test executables whole-sale. You could probably reuse some of the testing infrastructure (tasty-bench exists for a reason). If this sounds too much, you should just stop trying to achieve it because currently you're posting numbers for validate that most likely have no connection to the actual performance change, and as such are actively misleading. It'd be great to have some very simple benchmark that shows that we don't degrade performance significantly but other than that we should focus on design rather than performance, I think: the design issue has been a die-hard for a long time... |
hackage-tests looks for
I totally understand. I can make sure Otherwise, do you have an example of using tasty-bench to benchmark something similar in cabal? We wanted to add benchmark because in the tech proposal phase people were concerned about performance. |
This is the second part of the exact print parser. We changed the parser so that it doesn't merge the common stanzas during the parsing stage.
Depends on #11285 to update the test suite (after which the diff will be significantly smaller).
Background
Common stanzas are merged into their corresponding section during the parsing stage. This merging process doesn't retain the definition of the imports (i.e.
common <name> ...) nor where they are used (import: <name>). To achieve exact printing and to be able to programmatically update the import list, we need to retain this information.This PR is to remedy exactly that by deferring the merging process.
Implementation
We insert a new field
gpdCommonStanza :: Map ImportName (CondTree ConfVar [Dependency] BuildInfo)in theGenericPackageDescriptiondata type to track all the defined common stanzas. The merging is run only when the new accessors are called on aGenericPackageDescription.We also added a bidirectional
PatternSynonymthat uses the old record syntax. The impact on existing code should be very low. Comparing against my PR to update the test suite is promising leana8959/cabal@accessor-tests...leana8959:cabal:no-flatten-package-description.Please let me know your thoughts :)
Checklist below:
This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.